fix: resolve conditional React Hook calls in Link and Script shims#223
Conversation
Move early returns after all hook calls to satisfy the Rules of Hooks. - link.tsx: the dangerous-scheme early return was before useState/useRef/ useEffect/useCallback/useMemo — moved it to after all hooks - script.tsx: the SSR early return was before useEffect — moved useEffect before the check since it never runs during SSR anyway
|
/bigbonk review here please |
commit: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| // Apply locale prefix if specified (safe even for dangerous hrefs since we | ||
| // won't use the result when isDangerous is true) | ||
| const localizedHref = applyLocaleToHref( | ||
| isDangerous ? "/" : resolvedHref, |
There was a problem hiding this comment.
The substitution of "/" for the dangerous href is a reasonable way to keep applyLocaleToHref and withBasePath from operating on a javascript: string. However, this means localizedHref, fullHref, handleClick, and the prefetch useEffect all execute with a valid-looking "/" path when the href is actually dangerous.
This is fine today because:
- The early return at line 477 prevents the
<a>from having anhreforonClick/handleClickattached, so the prefetched"/"and the click handler are never used. - The
useEffectat line 317 will set up an IntersectionObserver for"/"and attempt to prefetch the root page — this is a harmless but unnecessary side effect.
Worth guarding the prefetch effect to avoid the wasted work:
| isDangerous ? "/" : resolvedHref, | |
| const localizedHref = applyLocaleToHref( | |
| isDangerous ? "/" : resolvedHref, | |
| locale, | |
| ); |
Actually, the substitution itself is fine — but consider also skipping prefetch setup when isDangerous is true. You could add !isDangerous to the shouldPrefetch condition, or add an early return inside the prefetch useEffect. Minor nit though, not a blocker.
| if (process.env.NODE_ENV !== "production") { | ||
| console.warn(`<Link> blocked dangerous href: ${resolvedHref}`); | ||
| } | ||
| return <a {...anchorProps}>{children}</a>; |
There was a problem hiding this comment.
Small behavioral difference from the original code: the old early return used restWithoutLocale (minus passHref) for the inert <a>, while this now uses anchorProps which is also restWithoutLocale minus passHref (defined at line 469). So the spread is equivalent — good.
One thing the old code did not pass through was ref and onClick. The new code also omits those (no ref={setRefs}, no onClick={handleClick}) which is correct for an inert element. Looks good.
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| // Client path: load scripts via useEffect based on strategy. | ||
| // useEffect never runs during SSR, so it's safe to call unconditionally. | ||
| useEffect(() => { |
There was a problem hiding this comment.
This is the cleanest fix of the two. useEffect never fires during SSR, so moving it before the typeof window === "undefined" check is a no-op in terms of runtime behavior while satisfying the Rules of Hooks. The removed // eslint-disable-next-line react-hooks/rules-of-hooks suppression is also a good sign — the lint rule now passes without it.
Nicely done.
|
Review posted. Here's a summary of my feedback on PR #223: Overall: The PR correctly identifies and fixes two Rules of Hooks violations. Both changes are sound. link.tsx: The approach of replacing the dangerous href with script.tsx: This is the cleaner of the two fixes. Since |
2 participants
Summary
useState,useRef,useEffect,useCallback,useMemo), making them conditional. Moved the check to a boolean flag before hooks and the early return to after all hooks.typeof window === "undefined") was beforeuseEffect, making it conditional. MoveduseEffectbefore the SSR check since it never runs during SSR anyway.Test plan
<Link>(e.g.javascript:hrefs render inert<a>)<Script>SSR rendering still works forbeforeInteractivestrategy